Skip to content

Close ProcessOutputStream in Runtime::eval_php_code_in_subprocess#265

Merged
ashfame merged 1 commit intoWordPress:trunkfrom
fredrikekelund:f26d/close-php-subprocess-stream
May 7, 2026
Merged

Close ProcessOutputStream in Runtime::eval_php_code_in_subprocess#265
ashfame merged 1 commit intoWordPress:trunkfrom
fredrikekelund:f26d/close-php-subprocess-stream

Conversation

@fredrikekelund
Copy link
Copy Markdown
Contributor

@fredrikekelund fredrikekelund commented May 5, 2026

Codex helped me diagnose and fix this issue.

When running the following blueprint using php-cli on Windows, I ran into an error like this:

View blueprint 
{
	"$schema": "https://playground.wordpress.net/blueprint-schema.json",
	"landingPage": "/wp-admin/themes.php",
	"steps": [
		{
			"step": "installTheme",
			"themeData": {
				"resource": "wordpress.org/themes",
				"slug": "twentytwentyone"
			}
		},
		{
			"step": "activateTheme",
			"themeFolderName": "twentytwentyone"
		}
	]
}
View logs 
{"type":"error","message":"Failed to remove directory: C:\/Users\/fredrik\/AppData\/Local\/Temp\/wp-blueprints-runtime-69f9bf3281ae4\/tmp_69f9bf32b8d62","details":{"exception":"WordPress\\Filesystem\\FilesystemException","message":"Failed to remove directory: C:\/Users\/fredrik\/AppData\/Local\/Temp\/wp-blueprints-runtime-69f9bf3281ae4\/tmp_69f9bf32b8d62","file":"phar:\/\/C:\/Users\/fredrik\/Documents\/GitHub\/studio\/apps\/studio\/out\/Studio-win32-arm64\/resources\/cli\/wp-files\/blueprints\/blueprints.phar\/components\/Filesystem\/class-localfilesystem.php","line":157,"trace":"#0 phar:\/\/C:\/Users\/fredrik\/Documents\/GitHub\/studio\/apps\/studio\/out\/Studio-win32-arm64\/resources\/cli\/wp-files\/blueprints\/blueprints.phar\/components\/Filesystem\/Mixin\/trait-rmdirrecursive.php(21): WordPress\\Filesystem\\LocalFilesystem->rmdir_single('C:\/Users\/fredri...', Array)\n#1 phar:\/\/C:\/Users\/fredrik\/Documents\/GitHub\/studio\/apps\/studio\/out\/Studio-win32-arm64\/resources\/cli\/wp-files\/blueprints\/blueprints.phar\/components\/Filesystem\/Mixin\/trait-rmdirrecursive.php(15): WordPress\\Filesystem\\LocalFilesystem->rmdir('C:\/Users\/fredri...', Array)\n#2 phar:\/\/C:\/Users\/fredrik\/Documents\/GitHub\/studio\/apps\/studio\/out\/Studio-win32-arm64\/resources\/cli\/wp-files\/blueprints\/blueprints.phar\/components\/Filesystem\/Layer\/class-chrootlayer.php(103): WordPress\\Filesystem\\LocalFilesystem->rmdir('C:\/Users\/fredri...', Array)\n#3 phar:\/\/C:\/Users\/fredrik\/Documents\/GitHub\/studio\/apps\/studio\/out\/Studio-win32-arm64\/resources\/cli\/wp-files\/blueprints\/blueprints.phar\/components\/Blueprints\/class-runner.php(291): WordPress\\Filesystem\\Layer\\ChrootLayer->rmdir('C:\/Users\/fredri...', Array)\n#4 phar:\/\/C:\/Users\/fredrik\/Documents\/GitHub\/studio\/apps\/studio\/out\/Studio-win32-arm64\/resources\/cli\/wp-files\/blueprints\/blueprints.phar\/components\/Blueprints\/bin\/blueprint.php(398): WordPress\\Blueprints\\Runner->run()\n#5 phar:\/\/C:\/Users\/fredrik\/Documents\/GitHub\/studio\/apps\/studio\/out\/Studio-win32-arm64\/resources\/cli\/wp-files\/blueprints\/blueprints.phar\/components\/Blueprints\/bin\/blueprint.php(675): handleExecCommand(Array, Array, Array, Object(JsonProgressReporter))\n#6 C:\\Users\\fredrik\\Documents\\GitHub\\studio\\apps\\studio\\out\\Studio-win32-arm64\\resources\\cli\\wp-files\\blueprints\\blueprints.phar(12): require('phar:\/\/C:\/Users...')\n#7 {main}"}}

This PR explicitly closes the ProcessOutputStream in Runtime::eval_php_code_in_subprocess before returning. I've confirmed that this fixes the issue on my Windows VM.

@fredrikekelund
Copy link
Copy Markdown
Contributor Author

fredrikekelund commented May 5, 2026

I'd greatly appreciate a review on this, @adamziel or maybe @ashfame 🙏

@fredrikekelund fredrikekelund mentioned this pull request May 5, 2026
Merged
1 task
@fredrikekelund
Copy link
Copy Markdown
Contributor Author

fredrikekelund commented May 6, 2026

The PHPUnit Tests / PHP 7.4 / windows-latest CI failure looks like a fluke.

fredrikekelund added a commit to Automattic/studio that referenced this pull request May 7, 2026
@fredrikekelund
Make native-php E2E tests pass on Windows (#3343)
97c6f10 
## Related issues

<!--
Link a related issue to this PR. If the PR does not immediately resolve
the issue,
for example, it requires a separate deployment to production, avoid
using the "Fixes" keyword and use "Related to" instead.
-->

- Fixes RSM-2326

## How AI was used in this PR

<!--
Help reviewers understand what to look for and verify that you've
reviewed the code yourself.
-->

Codex was used to help diagnose race condition issues in
`collectDirectoryMetadata` and to fix an issue in `blueprints.phar`
given error logs from a local Windows VM. I also used it to add the
`native-php` E2E testing config for Buildkite. Claude was used to debug
the persistently failing E2E tests and came up with the OPcache
directory fix.

## Proposed Changes

- Fix an OPcache configuration issue that was causing some E2E tests to
fail on Windows.
- Kill the PHP child processes spawned by `php-server-child.ts` on
Windows, too.
- Temporarily include a precompiled `blueprints.phar` with a bugfix
needed to make `blueprints.test.ts` pass. This can be removed when
WordPress/php-toolkit#265 has landed and there's
a new php-toolkit release. I advise landing this PR first and reverting
this change later.
- Make `collectDirectoryMetadata` more robust against race conditions.

## Testing Instructions

<!--
Add as many details as possible to help others reproduce the issue and
test the fix.
"Before / After" screenshots can also be very helpful when the change is
visual.
-->

E2E tests should pass on both platforms.

## Pre-merge Checklist

<!--
Complete applicable items on this checklist **before** merging into
trunk. Inapplicable items can be left unchecked.

Both the PR author and reviewer are responsible for ensuring the
checklist is completed.
-->

- [ ] Have you checked for TypeScript, React or other console errors?
@ashfame
Copy link
Copy Markdown
Member

ashfame commented May 7, 2026

@fredrikekelund Thanks for the fix! Re-running that failed CI job now made it green 👍

@ashfame ashfame merged commit 3623d01 into WordPress:trunk May 7, 2026
55 of 56 checks passed
@ashfame ashfame mentioned this pull request May 7, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fredrikekelund @ashfame